[BUG] add raise of softdep not working when we install all extras#3409
[BUG] add raise of softdep not working when we install all extras#3409baraline wants to merge 5 commits into
Conversation
baraline
changed the title
Thank you for contributing to
|
baraline
added
testing
|
we had this issue 2 years ago with tensorflow-addons, where it was actually a _ instead of -, and so the tests were not running without us knowing :) |
|
If we are OK with it, we can get this one in to check for that while we discuss a better solution. It is supposing we have all soft deps on ubuntu, but for now it is the case. This way we can move forward with the MONSTER data issue. |
There was a problem hiding this comment.
Go with 3.11 to avoid the mrseql and mrsqm timebombs for now.
Is there a way to do this as its own test without actually running anything? e.g. install everything, collect all tests then fail skils without running non-skips.
Fine with this going in if you want to link an issue to revisit later.
| skip_reason = ( | ||
| report.longrepr[2] | ||
| if isinstance(report.longrepr, tuple) | ||
| else str(report.longrepr) | ||
| ) |
There was a problem hiding this comment.
pretty evident what the function is doing as a whole but coud you leave a comment on why this is needed if possible
There was a problem hiding this comment.
Added some comments/docs any better ?
There was a problem hiding this comment.
specifically meant that bit of code, im not really sure what in a test could cause the output type the change! other bits seem reasonable to understand just reading through
|
There is definitly a cleaner way to do this, but we need to plan for it properly, so I think linking an issue and listing our options is the way to go here. |
|
This doesnt seem to interact well the current tests due to some classifiers missing from all_extras (which in itself it a bit of a hack due to annoying packages, but thats another topic). I can try make a PR into this branch with its own test if you don't want to take that on. Alternatives involve removing whole estimators which is probably a good idea but takes too long for this case. |
|
Could also hack it to ignore specific estimators, hacks within hacks |
3 participants
Reference Issues/PRs
Related to #3406
What does this implement/fix? Explain your changes.
While running tests in CI on environment where we install all_extras, we didn't raise when softdep checks failed, as they had severity=None in the test files. Added a flag to make these test raise.
One issue is that it's kind of the point of the softdeps checks, as some of these are OS dependent. But the linked issue showed that it can hide real failures. So with this PR we basically say, we trust one OS (here ubuntu) to be able to take all softdeps.
This is more of a quickfix. Maybe a better future solution would be to label those test under different pytest marks (e.g. @pytest.mark.softdepschecks with additional marks for specific OS) that would only run on these CI environment using all_extras.
This PR CI should fail due to the "hugging-face" softdep check.